fix(studio): claim upload filenames exclusively - #3786
Conversation
miguel-heygen
left a comment
There was a problem hiding this comment.
Exact-head security/collision review at 68735f002d1db2534360e4ffc34d8cd67eb6abb6.
packages/studio-server/src/routes/files.ts:2201-2226preserves the existing naming contract and checks both the initial and preselected suffix destination. Dotfiles keep no extension; compound extensions remain attached after the suffix.:2235-2255makeswriteFileSync(..., { flag: "wx" })the only publication operation. EveryEEXISTadvances exactly once and re-entersisSafePathbefore the next attempt;(9999)remains the last attempted suffix under the existing<10000cap. Non-collision write failures still propagate.- The symlink states stay distinct: existing or newly planted outside/dangling leaves fail containment and are skipped; regular/internal collisions reach
wx, cannot be followed or overwritten, and advance to the next name. The remaining parent/ancestor swap window is the disclosed trusted-ancestor boundary, not widened by this patch. :2257-2260records and waveforms only the filename whose exclusive write succeeded. Validation remains before publication and suffixing never changes the extension it validates.- All four new write-time race cases pass in the focused local run. A separate direct probe against this head’s
isSafePathsource confirms live outside and dangling leaves are rejected; the borrowed prebuilt dependency environment was stale for two older assertions, so exact-head CI remains authoritative for the complete 16-case suite.
No blocking findings. Build, Typecheck, runtime contract, lint, format, Studio load/timeline, and regression are green; required Test, Windows, and JavaScript CodeQL were still running at review time and remain merge gates.
Verdict: APPROVE
Reasoning: Exclusive leaf creation closes the check/use overwrite race while preserving collision names, suffix bounds, validation, response, skip, and waveform semantics; each retry revalidates the only pathname property the patch claims to protect.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
The descriptor lifecycle is correct: fd is block-scoped to a successful openSync, writeFileSync(fd, buffer) is enclosed by finally, closeSync(fd) runs before written can become true, and non-EEXIST failures still propagate. The collision path is also unchanged: only EEXIST advances the suffix once, the containment check still precedes each attempt, and the existing bound remains intact.
One blocker remains: the exact-head CodeQL run completed failure and opened alert #900, again js/file-system-race, now at the new openSync(finalPath, "wx") call. Alert #899 is fixed, but this is a fresh instance of the same query at the same operation. Since this revision explicitly intends to clear the alert without suppression or dismissal, it has not yet achieved that acceptance condition and the native security gate remains red. Please restructure the check/open flow so the query closes while retaining the unsafe-link skip behavior, or explicitly revisit the no-dismissal constraint with a scoped audit of the remaining finding.
Separately, the required Typecheck failure is the known generated-runtime build-order race (runtime-inline.ts was read mid-write, TS1002: Unterminated string literal), not caused by these two files; it needs a retry but is not a code-review blocker here.
Verdict: REQUEST_CHANGES
Reasoning: Descriptor closure and retry behavior are sound, but exact-head CodeQL still raises the same race class as a new blocking alert, so the security fix does not yet clear its stated gate.
— Magi
miguel-heygen
left a comment
There was a problem hiding this comment.
Scoped audit of CodeQL #900 at unchanged head 63992e193c231306416089158b2c191b5fbbca9a: false positive within this PR's claimed leaf-race boundary. I recommend an individual false-positive dismissal; I did not dismiss it.
packages/studio-server/src/routes/files.ts:2213is only a filename preselection hint. A stale false result does not authorize a path write::2241opens withwx, and only that successful open can produce the fd used at:2243. A leaf that appears in the window yieldsEEXIST;:2251-2253advances the suffix and the loop rechecks containment before the next attempt. A stale true result can at worst choose a now-unnecessary suffix.- Node documents
wxas create-for-write that fails when the path exists and mapsO_CREAT|O_EXCLto WindowsCREATE_NEW; Node's own recommended race-free example isopen(..., 'wx')followed by descriptor I/O. On Linux,straceagainst the repo's Node 22 showedO_WRONLY|O_CREAT|O_EXCL|O_TRUNC, and POSIX/Linux specify the existence-check/create as atomic and requireEEXISTfor a symlink leaf regardless of its target. - Linux control: existing regular, live symlink, and dangling symlink all returned
EEXIST; the victim bytes and missing dangling target were unchanged. Across 50 synchronized 16-writer races, every round produced exactly one creator and 15EEXISTs. Five hundred open/write/close cycles left/proc/self/fdunchanged (22 -> 22). - Windows control is contractual rather than inferred: Node 22 uses libuv; libuv 1.51 maps
UV_FS_O_CREAT|UV_FS_O_EXCL(including the truncating form produced bywx) toCREATE_NEW, and Microsoft specifies thatCREATE_NEWfails withERROR_FILE_EXISTSwhenever the named leaf exists and creates only an absent leaf. The exact-head Windows workflow is green. - The fd is block-scoped and closed in
finally; after the exclusive open, payload writes do not resolve the pathname again. No regular-file, hard-link, live-link, or dangling-link race can redirect or overwrite an existing leaf through this source-to-sink path.
The residual race is in ancestor path-component replacement between isSafePath and openSync, plus Node's documented caveat for filesystems that do not honor exclusive creation. This PR explicitly leaves trusted ancestors unchanged, and the Studio/local-filesystem contract plus current ext filesystem are inside that boundary; neither is #900's reported leaf race.
The retried Typecheck, Test, and Windows suites are green at this head. CodeQL remains red solely pending disposition of #900. This approval supersedes my prior request-changes after the scoped audit changed the acceptance path.
Verdict: APPROVE
Reasoning: wx makes leaf creation authoritative and atomic on the supported Linux/Windows implementations, descriptor I/O removes the post-open pathname use, and the alert's precheck can affect naming only—not overwrite or redirection.
— Magi
Problem and change
Fixes CodeQL #694. Studio chooses an unused upload filename before awaiting the uploaded bytes, so another upload can create that path before the write and have its contents overwritten.
Open the selected filename with exclusive
wxcreation, then write through and close that descriptor. OnEEXIST, retry the existing numbered suffix convention up to the same 10000 boundary. Revalidate containment before each attempt, including after the asynchronous read. Only a successfully written filename is returned or passed to waveform generation.Preserves extension/dotfile naming, ordinary collision choices, media validation, size limits, skipped/invalid response shapes, and waveform triggering. Unsafe symlink paths remain skipped. This protects destination leaves; existing trusted-project ancestor assumptions and same-inode concurrent mutation remain unchanged. No workflow changes.
Validation